Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: optional commit-lints #53

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Conversation

manuel-bertoluzza-zupit
Copy link
Contributor

@manuel-bertoluzza-zupit manuel-bertoluzza-zupit commented Oct 11, 2023

This PR introduces the option to make "Lint commits" job optional.

In repositories where the "squash and merge" strategy is used for merging pull requests, commits are squashed in every branch after merge.
Linting commits that in main are lost after the PR has been merged is not very useful.

The change is backwards compatible, because by default "Lint commits" continues to run as it does currently.


Checklist before requesting a review

  • I have performed a self-review of my code
  • Does the code follow the team's coding standards and best practices?
  • Is the code well-organized and easy to understand?
  • Are file, variable and function names clear and meaningful?
  • Are there any duplicated code blocks that can be refactored into reusable functions?

Functionality

  • Does the PR implement the intended functionality or fix the issue described in the title and description?
  • Are there any edge cases that have been overlooked?

Testing

  • Are there sufficient tests covering the changes?
  • Are there any missing test cases that need to be added?
  • Has the code been tested in different environments if applicable (e.g., browsers, platforms, devices)?

Documentation

  • Are the changes well-documented and easy to understand?
  • Are there any missing or outdated comments in the code?
  • Is the PR title clear and detailed enough to provide necessary context?

Performance

  • Do the changes have any impact on the system's performance?
  • Are there any potential bottlenecks that need to be addressed?

Dependencies

  • Have the changes introduced any new dependencies or updated existing ones?
  • Are the dependencies up-to-date and secure?

Compatibility

  • Will the changes have any adverse effects on other parts of the system?
  • Do they work well with other existing features?

Continuous Integration/Deployment (CI/CD)

  • Have the changes passed the CI/CD pipeline?
  • Are there any deployment considerations or steps that need to be taken into account?

Code Review language

  • Is the feedback provided in a constructive and respectful manner?
  • Has the reviewer avoided personal attacks or harsh language?
  • Are there any suggestions for improvement or alternative approaches?

@alessio-libardi-zupit
Copy link
Member

I'm sad I wasn't tagged here. My honest opinion is that we should just remove the job.

Linting commits that in main are lost after the PR has been merged is not very useful.

Indeed that's useless, so why not just removing the job instead of adding complexity with a flag?

@omar-muscatello-zupit
Copy link
Contributor

We agree with @alessio-libardi-zupit to remove the both the job itself. @manuel-bertoluzza-zupit would you mind updating your code?

@manuel-bertoluzza-zupit
Copy link
Contributor Author

I removed lint-commits job as requested

@omar-muscatello-zupit omar-muscatello-zupit merged commit 8f706b2 into main Feb 12, 2024
1 check passed
@omar-muscatello-zupit omar-muscatello-zupit deleted the feat/optional-lint-commits branch February 12, 2024 11:19
alessandro-mariotti-zupit pushed a commit that referenced this pull request Feb 12, 2024
# [1.13.0](v1.12.0...v1.13.0) (2024-02-12)

### Features

* optional commit-lints ([#53](#53)) ([8f706b2](8f706b2))
@alessandro-mariotti-zupit
Copy link
Member

🎉 This PR is included in version 1.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants